Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add constant for invalid surface tokens. #3260

Merged
merged 10 commits into from
Jan 16, 2025

Conversation

pshriwise
Copy link
Contributor

@pshriwise pshriwise commented Jan 12, 2025

Description

As I was going through some of the plotting code I noticed that through out the code we use a value of 0 for an invalid surface token. Here, token meaning a signed, off-by-one index as opposed to a signed surface ID.

The "magic number" is a little confusing when we're constantly converting between tokens and indices into the model::surfaces vector. Further, we initialize BoundaryInfo.surface_index to 0 because this member is really a token rather than an index.

In an effort to distinguish surface token values I've added a SURFACE_NONE constexpr that is 0 for these cases.

I've also updated BoundaryInfo::surface_index attribute to surface in an effort to be consistent with the GeometryState::surface_ attribute and added BoundaryInfo|GeometryState::surface_index methods that convert from the token to the index for that token. This doesn't cover all of the cases where we do this conversion, but I think it generally increases clarity were surface indicates a token and surface_index an index.s

Update

It's worth noting that there's a fix for the surface index passed into the DAGMC next_cell call for projection plots. The off-by-one used to be handled in next_cell but the argument passed in was already adjustment, so this was happening twice.

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable) N/A
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable) N/A

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the change here! One question though about the use of next_cell for DAGMC geometries:

src/dagmc.cpp Show resolved Hide resolved
@paulromano paulromano merged commit 32662b4 into openmc-dev:develop Jan 16, 2025
16 checks passed
@pshriwise pshriwise deleted the no-surface-value branch January 17, 2025 19:34
pshriwise added a commit to gridley/openmc that referenced this pull request Jan 17, 2025
@pshriwise pshriwise mentioned this pull request Jan 17, 2025
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants